Thanks for this contribution. It looks like a useful feature.

I will be happy to test it in due course, on Linux, Windows and Mac, but at the moment most of my kit is packed away for a pending house move.

@softins thanks - no worries, I guess there will be plenty of time for testing before this ever makes it in (if it does).

Hope the house move goes smoothly.

Nice!

I'd like to open a discussion:

1. Do you think a separate MIDI tab is useful or does it add clutter?
2. Is there a better word instead of "Learn" for the detector? How can we make the feature clear to the user? Would a dynamic text field showing currently pressed buttons be better? (I'm unsure as this could be more complicated)

@ann0see

1. Given the number of MIDI settings, I don't see how it would fit in somewhere else without cluttering up that section in a worse way. I think it merits its own tab.
2. "Learn" is a standard term for this feature. I see and use it in many other MIDI applications. Maybe "MIDI learn"? But I think that might make the button too large.

In any event, this is still under development as I'm finding that some of my implementations are far from perfect. E.g. the way the MIDI in port is currently kept open I think has been done wrong, so I'm working on that. I'm testing a version where the MIDI port can be enabled/disabled at runtime, as among other things it affects whether numbers are prepended to user names in the mixer board - if you're not using MIDI you won't want that.

The main purpose for opening this PR was to generate builds for other platforms for testing, but for now it's just a PoC that's far from finished, so any suggestions are welcome.

Tested on Linux, Windows (no JACK) and macOS (legacy and non-legacy), and it seems to be working on these three platforms at least.

OK just stumbled onto this. As this is a new feature, the following need to be adhered to:

• All updates in the UI should only work by emitting a signal to the main application, where state is held -- no state to be held in the UI (I've not reviewed the code yet)
• Any UI enhancement should be mirrored by an enhancement to the JSONRPC API, if the feature is missing there

(The "learn" feature for JSONRPC would just be the eventual "set X to Y" update, the visual behaviour would be for the JSONRPC user to supply - it just needs to be able to send the same update to the core code as the Client UI.)

Get rid of all the unnecessary changes in spacing and resubmit the PR. As it stands, I can't review it.

Done. Those keep getting added in by clang.

All updates in the UI should only work by emitting a signal to the main application, where state is held -- no state to be held in the UI (I've not reviewed the code yet)

It's been almost a year since I did this, but as far as I recall it updates the main application state via pSettings and pClient.

I believe this should be rebased and squashed into one commit.

It's been almost a year since I did this, but as far as I recall it updates the main application state via pSettings and pClient.

It looks pretty good -- as @ann0see says, please rebase and squash. Then check the resultant diff for anything obvious 😄 - it looks mostly okay to me but there's a few places I'm not sure what was being intended, so I might have some more questions.

One thing I might ask is for the command line parsing code to be moved into soundbase.cpp (as a static), just to remove a bit of clutter.

as @ann0see says, please rebase and squash.

Done.

One thing I might ask is for the command line parsing code to be moved into soundbase.cpp (as a static), just to remove a bit of clutter.

Done. I don't know what clang is whining about - I can't use it because it reformats a ton of stuff that shouldn't be.

I don't know what clang is whining about - I can't use it because it reformats a ton of stuff that shouldn't be.

Known issue. We should really bump the clang format version. Otherwise be sure to use the same clang format version as used in the CI

• Any UI enhancement should be mirrored by an enhancement to the JSONRPC API, if the feature is missing there

I've been working on this - should the UI immediately reflect changes from JSONRPC commands sent? It doesn't seem to do this for the existing commands but just to confirm.

Known issue. We should really bump the clang format version. Otherwise be sure to use the same clang format version as used in the CI

The problem is that in the TODO blocks, the TODO lines don't have a space after //, but all other comments do. I couldn't find a way to preserve that distinction other than by adding CommentPragmas: '^ TODO:|^### TODO:' to clang-format. It works (I'll use it myself for now), but maybe there's a better way.

Ok, done. Each control can be enabled/disabled and omitting them from the command line disables and resets them to default values (overwriting the ini file). This was the behaviour before, but maybe just disabling them is enough?

Only tested in Linux, but seems to work ok. Here's what it looks like now:

There are quite a few comments in the code to help me keep track of what's going on.

If the command line is specified at all, then nothing should be written back to settings, IIRC? Not that there's any examples for the client but I think the server works like that (if you run the GUI and specify a Directory to register with, then change it in the UI, I don't think it gets updated in settings). As I've mentioned, there's a lot of discussion about how that should work...

To me, if I've set everything in the UI and then as a one off run with --ctrlmidich from the command line, I don't want the settings I did in the UI changed.

And I probably want the saved settings for any --ctrlmidich value not specified to be the UI defaults.

Windows - JACK and MME versions. Both run with --ctrlmidich "1;f85*3;p94*3;dnanoKONTROL", which was unfortunate as JACK doesn't pick the MME names up. I think it also doesn't reliably associate capture_N with any particular port, so it's a bit hit and miss. But it looks like it would work, anyway!

As you can see, though, the spinners are much too small -- they all let only a single digit show. That's a Qt issue, though -- Qt 6 widgets aren't the same as Qt 5.

(Approximation again in markdown...)

and make sure the width on the spinner and its container can expand as needed.

1. Add

   [remote "upstream"]
           url = git@github.com:jamulussoftware/jamulus.git
           fetch = +refs/heads/*:refs/remotes/upstream/*
   

   to your .git/config
2. Check out main
3. git pull upstream main
4. git push
5. Checkout midi-gui-learn
6. git rebase main
7. git push --force

That should fix your branch iOS build. You should keep upstream in your config and pull from there to main to keep your fork up to date.

Qt 6.10.2 on Linux looks fine. It's ridiculous that Qt can't manage the UI tools so you can write the layout and know it's going to render okay cross platform. I mean, that's the point of Qt...

If the command line is specified at all, then nothing should be written back to settings, IIRC? Not that there's any examples for the client but I think the server works like that (if you run the GUI and specify a Directory to register with, then change it in the UI, I don't think it gets updated in settings). As I've mentioned, there's a lot of discussion about how that should work...

To me, if I've set everything in the UI and then as a one off run with --ctrlmidich from the command line, I don't want the settings I did in the UI changed.

And I probably want the saved settings for any --ctrlmidich value not specified to be the UI defaults.

I agree. But after having a look, the server settings actually behave more or less the way the MIDI settings do now: they skip reading the ini file if the command line option is present and later write any command line-specified values to the ini file later. What to do with non-specified controls in MIDI commands is a different matter though because AFAICT it's the only command line option that has a number of optional parameters within it, so there's not really a precedent for that scenario. I agree that they should be left alone and at most just disabled, not overwritten with defaults. Though this requires changing the current approach where the command line is checked first, to reading the xml and then checking the command line. The latest version does this:

• Any setting specified on the command line overrides what's in the ini file and is saved as such.
• Settings not specified are disabled but their values are preserved. Their "disabled" status is written to the ini file.
  In short - launching and setting values with the command line is treated as though done through the GUI, and if you omit a control parameter, it's assumed you don't wish to use it.

I also fixed a bug where the "Learn" buttons for disabled controls would become active after using Learn for an active control.

As you can see, though, the spinners are much too small -- they all let only a single digit show.

It's those stupid up/down arrows that take up so much space. Please check the latest version to see if it's fixed.

Oh, and another thing - please test launching a server. After years of being able to launch and connect my own, my bl**dy ISP has changed its policy and now I'm behind CGNAT and no IPv6 available, so I can't test that. It should be fine, but just in case.

Oh, and another thing - please test launching a server. After years of being able to launch and connect my own, my bl**dy ISP has changed its policy and now I'm behind CGNAT and no IPv6 available, so I can't test that. It should be fine, but just in case.

I've no IPv6, either. IPv4 registered okay with Any Genre 1 and one of my local Directories. Local connection worked fine.

I'm seeing

QThreadStorage: entry 3 destroyed before end of thread 0x6520f4497380
QThreadStorage: entry 2 destroyed before end of thread 0x6520f4497380
QThreadStorage: entry 1 destroyed before end of thread 0x6520f4497380

from the client (Audio, UI and Client threads, I think) and

QThreadStorage: entry 2 destroyed before end of thread 0x5b1062a0d380
QThreadStorage: entry 1 destroyed before end of thread 0x5b1062a0d380

from the server (UI and Server threads) when the UI shuts down normally. @softins any idea what this is about?

... I'm vaguely remembering it was mentioned somewhere before...

Stupid widget. It's probably Windows rather than Qt, to be fair -- I think Qt tries to use the underlying platform widgets. But you do end up not having any idea what's going to render okay. The spinner still isn't wide enough...

I'm not sure why the Mute Myself channel doesn't align - I guess it's padding to available space.

Do you try GridLayout? You can stick one inside a Checkable Groupbox (i.e. make the "MIDI-in" a group box around everything, replacing the one around the set of learn-ables) with a blank set of cells between each "row". You can span just by dragging a widget and set alignment on a cell.

Otherwise vertical and horizontal alignment end up a bit hit and miss (see how "Musician Profile" and "User Interface" end up looking so clumsy on the "My Profile" tab)

I'm seeing

QThreadStorage: entry 3 destroyed before end of thread 0x6520f4497380
QThreadStorage: entry 2 destroyed before end of thread 0x6520f4497380
QThreadStorage: entry 1 destroyed before end of thread 0x6520f4497380

from the client (Audio, UI and Client threads, I think) and

QThreadStorage: entry 2 destroyed before end of thread 0x5b1062a0d380
QThreadStorage: entry 1 destroyed before end of thread 0x5b1062a0d380

from the server (UI and Server threads) when the UI shuts down normally. @softins any idea what this is about?

I've seen it too. I think it's been around for a while, but as it only happens when exiting, it doesn't seem to be a problem. I have never investigated the cause.

my bl**dy ISP has changed its policy and now I'm behind CGNAT and no IPv6 available, so I can't test that.

Urgh.

But you might be able to use Hurricane Electric tunnels to enable IPv6.

I'd open a support ticket with the ISP saying something like your "connectivity is broken for IPv6 and you need it".

@gilgongo, as our resident UI/UX arbitrator, what do you reckon? Is it okay to go with this?

AI put me out of a job there, it seems :-)

BTW regarding, "if I disable a control temporarily, I don't want to have to go through the process of setting the value again to get it working" - this is something I used to summarise as "user input is sacred". The ideal app never discards anything you do, or prevents you from undoing what you did. This can of course lead to technical complications, but by far and away the worst UX is lost work (even if that "work" is seemingly trivial like having to re-type a digit).

Do you try GridLayout? You can stick one inside a Checkable Groupbox (i.e. make the "MIDI-in" a group box around everything, replacing the one around the set of learn-ables) with a blank set of cells between each "row". You can span just by dragging a widget and set alignment on a cell.

Followed this suggestion and I think it's better now. Please check on Windows 11 to see if we're there yet with the spinboxes (I added minimum width).

The ideal app never discards anything you do

It's tricky to find a balance between consistency with current behaviour (command line overrides and overwrites previous settings) and not discarding previously saved (but not currently used) settings, so I think a compromise where omitted controls on the command line are disabled (not including them is taken as a wish not to use them) but values are preserved is acceptable.

I'd open a support ticket with the ISP saying something like your "connectivity is broken for IPv6 and you need it"

Rang them this morning. A customer for 20+ years and they wanted more money on top of an already extortionate price, so I talked to another ISP that will disable CGNAT and charge me 1/3 for the same package... Silly me for not checking sooner but at least a nice side-effect of this PR : )

The fields are wide enough now -- I'm still not keen on the layout with the "Mute Myself" box looking so different. The spinner and learn buttons should all be stacked above one another.

I could live with it, though. I think we should get the PR out of draft, anyway.

The Mute Myself box looks perfectly aligned on my Linux PC but I think I know what the issue is. I'll have a look at that and another couple of things I want to correct later on tonight, and then set it ready for review.

I think the alignment should be fixed now.

At the risk of stretching this too far, there's a bonus feature that I worked on last year and is ready to be added (or can wait): MIDI pickup mode.

This is a handy feature particularly if you use MIDI CC banks in your controller: you've set a fader or pan control to a certain value (let's say 100%) and change banks to use the same knob to nudge the fader or pan of another strip from say 30% to 10%. Without pickup mode that control will immediately jump to 100% before you can lower it, and with pickup mode enabled it will wait for you to match the position before moving.

I haven't included it in this PR but you can find builds here, and see the logic, which is chiefly in audiomixerboard.cpp, on my midi-pickup branch.

It's supported on the command line (add mp (for "MIDI pickup") - including it enables it, and if not it's disabled, like the other controls) and by JSONRPC.